Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Include external tables in get_columns_in_relation redshift adapter #2754

Merged
merged 3 commits into from
Sep 25, 2020
Merged

Include external tables in get_columns_in_relation redshift adapter #2754

merged 3 commits into from
Sep 25, 2020

Conversation

aiguofer
Copy link
Contributor

@aiguofer aiguofer commented Sep 14, 2020 •
edited
Loading

resolves #2753

Description

This PR adds columns from external tables to get_columns_in_relation in the Redshift adapter. I'd be happy to add tests and update CHANGELOG if it makes sense.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label Sep 14, 2020
@jtcohen6
Copy link
Contributor

We need to fix a few issues with Redshift integration tests based on a recent change they deployed. Jake is working on those fixes over here, in a PR we'll merge quite soon.

@beckjake
Copy link
Contributor

We also need to fix the default branch! It should probably be dev/kiyoshi-kuromiya, but this PR could probably be against dev/0.18.1 if we wanted.

@aiguofer
Copy link
Contributor Author

Great! happy to change the target of the PR, should I make it against dev/kiyoshi-kuromiya or dev/0.18.1?

I'll go ahead and add the CHANGELOG change as well, and happy to update the PR once those integration tests are merged (link took me to the report feature PR, I guess that's where those changes are?)

@aiguofer
Copy link
Contributor Author

Actually, don't see a dbt next section in CHANGELOG... do I need to add that or do you guys update the CHANGELOG on release? if the latter, might want to update the PR template :)

@beckjake
Copy link
Contributor

We should probably refer to it as the "Release TBD" version! Both 0.18.1 and dev/kiyoshi-kuromiya should have a line like that.

@jtcohen6 jtcohen6 mentioned this pull request Sep 15, 2020
Closed
@jtcohen6 jtcohen6 changed the base branch from dev/marian-anderson to dev/0.18.1 September 15, 2020 15:31
@aiguofer aiguofer force-pushed the include_external_tables_in_get_columns_in_relation branch from c2de4f2 to 4e860cd Compare September 15, 2020 20:43
@aiguofer aiguofer changed the base branch from dev/0.18.1 to dev/kiyoshi-kuromiya September 15, 2020 20:49
@aiguofer aiguofer force-pushed the include_external_tables_in_get_columns_in_relation branch from 60c7115 to 9c24fc2 Compare September 15, 2020 21:11
@aiguofer aiguofer changed the base branch from dev/kiyoshi-kuromiya to dev/0.18.1 September 15, 2020 21:11
@aiguofer
Copy link
Contributor Author

sorry for the spam... realized dev/kiyoshi-kuromiya is the future 0.19.0 version so rebased again against dev/0.18.1

beckjake
beckjake reviewed Sep 16, 2020
View reviewed changes
Copy link
Contributor

@beckjake beckjake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I'm not sure how we can effectively test this - do we need to create a spectrum table or something that we can query as a source in our test suite?

jtcohen6
jtcohen6 approved these changes Sep 25, 2020
View reviewed changes
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good in my local testing. I need to figure out a long-term plan for staging spectrum tables in our testing env, since it's also a prerequisite to testing the dbt-external-tables package. In the meantime, a bunch of our existing integration tests use adapter.get_columns_in_relation, so we should quickly find out about any regressions introduced here.

I'll wait until some of the open PRs have been merged, and then get this in before v0.18.1.

@jtcohen6 jtcohen6 merged commit 35f78ee into dbt-labs:dev/0.18.1 Sep 25, 2020
@jtcohen6 jtcohen6 mentioned this pull request Sep 25, 2020
Closed
5 tasks
@dataders dataders mentioned this pull request Mar 25, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beckjake beckjake beckjake left review comments

@jtcohen6 jtcohen6 jtcohen6 approved these changes

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

get_columns_in_relation does not include columns from external tables in Redshift
3 participants
@aiguofer @jtcohen6 @beckjake